Conversation
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
📝 WalkthroughWalkthroughThe PR adds a shared relay status model, a relay service base and factory, Windows and SMTP relay service implementations, status updates in the voice and audio modes, and console startup changes that route through the factory and render tune progress. ChangesRelay service lifecycle and status wiring
Sequence Diagram(s)sequenceDiagram
participant Program
participant RelayServiceFactory
participant RadioRelayService
participant RadioMode
Program->>RelayServiceFactory: Create(options, logger)
RelayServiceFactory-->>Program: RadioRelayService
Program->>RadioRelayService: StartAsync(cancellationToken)
RadioRelayService->>RadioMode: RunAsync(options, logger, token, status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resgrid.Relay.Engine/Voice/RadioMode.cs (1)
29-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't leave
LiveKitstuck on the last optimistic state.
RadioRelayServicesetsMutableStatus.LiveKit = ConnectionState.Connectingbefore entering this method, but this code only ever promotes it toConnected. Seat-limit exits, join failures, and normal shutdown all leave the status surface showing the previous connection state instead of a terminal one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Resgrid.Relay.Engine/Voice/RadioMode.cs` around lines 29 - 56, Update RadioMode.RunAsync so MutableStatus.LiveKit is always moved off Connecting and into a terminal state on every path. Right now it only sets status.LiveKit to Connected after a successful JoinAsync, but the seat-limit return, any join/connection failure, and the normal end of the method can leave the last optimistic state visible. Use the existing status parameter in RunAsync to set an appropriate disconnected/faulted state before each early return and in failure/shutdown handling around provider.CanConnectAsync, provider.GetChannelAsync, and manager.JoinAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Resgrid.Audio.Core/Radio/RadioBridge.cs`:
- Around line 139-142: Update RadioBridge so Receiving is only set when RX audio
is actually forwarded, not just when squelch is open. In the receive path around
SetReceiving/open and the AntiLoop check, move or gate the Receiving state
update so it happens only after the early-return conditions pass, or clear it
when _settings.AntiLoop suppresses forwarding while _transmitting is true. Use
the RadioBridge receive flow and SetReceiving as the key symbols to adjust.
In `@Resgrid.Audio.Relay.Console/Program.cs`:
- Around line 102-107: The relay service wrappers are ignoring the non-zero
status returned by the mode runners, so failures from methods like RunAsync on
AudioImportService, RecordRelayService, RadioRelayService, and
DispatchRelayService are being treated as success. Update the service wrapper
flow around RelayServiceBase.StartAsync / the mode RunAsync calls to capture the
returned int, and if it is non-zero, throw an exception so the service is marked
faulted and the process exits with a failure code instead of returning 0.
In `@Resgrid.Relay.Engine/Services/DispatchRelayService.cs`:
- Around line 20-24: `DispatchRelayService.ExecuteAsync` is ignoring the return
value from `DispatchVoiceMode.RunAsync`, so validation/configuration failures
are not marked as faults. Capture the return code from `RunAsync` and, when it
is non-zero, transition the relay/service state to faulted (or otherwise signal
failure through the existing state mechanism) before exiting `ExecuteAsync`. Use
the existing `MutableStatus`/`ConnectionState` flow in `DispatchRelayService`
and the `DispatchVoiceMode.RunAsync` return contract to keep behavior aligned
with `Program`’s `RelayServiceState.Faulted` handling.
In `@Resgrid.Relay.Engine/Services/RelayServiceBase.cs`:
- Around line 56-59: The catch in RelayServiceBase.ExecuteAsync is treating
every OperationCanceledException as a graceful stop, which can hide real faults.
Update the exception handling in RelayServiceBase so it only transitions to
RelayServiceState.Stopped when the service’s own cancellation token was actually
requested; otherwise let the failure flow to Faulted. Use the existing
ExecuteAsync flow and TransitionTo method to distinguish genuine shutdown
cancellation from dependency timeouts or other canceled operations, so the
console path in Program can surface failures correctly.
- Around line 46-48: Prevent re-entrant StartAsync calls on the same
RelayServiceBase instance by adding an explicit running/state check before
replacing _cts. Update StartAsync and the service lifecycle in RelayServiceBase
so a second start either returns early or throws a clear error when ExecuteAsync
is already active, and only create a new linked CancellationTokenSource after
confirming the service is not already running.
In `@Resgrid.Relay.Engine/Services/RelayServiceFactory.cs`:
- Line 20: The mode normalization in RelayServiceFactory should default blank or
whitespace values to smtp, not just null. Update the logic around the mode
assignment so that options.Mode is checked for null, empty, or whitespace before
trimming/lowering, and fall back to smtp in those cases; keep the existing
unsupported-mode handling for real values, using the mode variable in
RelayServiceFactory as the central lookup point.
In `@Resgrid.Relay.Engine/Services/SmtpRelayService.cs`:
- Around line 45-47: The Redis status in SmtpRelayService is being set to the
transitional Connecting state without any follow-up update, so the status
surface can stay stuck. Update the MutableStatus.Redis assignment in
SmtpRelayService to either perform an actual Redis probe and set a final state
such as Connected or Disconnected, or keep it non-transitional (for example
NotApplicable) until StatusReportingSmtpTelemetry or another Redis signal can
provide a real result.
In `@Resgrid.Relay.Engine/Voice/AudioImportMode.cs`:
- Around line 39-41: The Resgrid API connection state is being marked as
Connected too early in AudioImportMode, before the health check completes.
Update the flow in the code around IsConnectionValid() so status.ResgridApi is
only set to ConnectionState.Connected after the health check succeeds, and keep
it unset or non-connected when validation fails; apply the same correction
wherever this connection-status assignment appears in the import path.
In `@Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs`:
- Around line 55-59: Don’t set TTS to Connected immediately after constructing
ResgridTtsClient in DispatchVoiceMode because the constructor does not verify
the service is reachable. Move the connection-state update to the first
successful synthesis/health-check path in DispatchVoiceMode, using the
ResgridTtsClient instance only after a real network call succeeds, and keep
status.Tts unchanged until that check passes.
In `@Resgrid.Relay.Engine/Voice/RadioMode.cs`:
- Around line 68-80: The TX/RX status wiring in RadioMode is happening too late
because the TxRxChanged subscription is added after RadioBridge.StartAsync, so
the initial transmitting/receiving state can be missed. Move the status handler
setup before calling bridge.StartAsync, or explicitly seed status.Transmitting
and status.Receiving from the bridge’s current state right after startup. Use
the RadioMode method and RadioBridge.TxRxChanged/StartAsync as the key symbols
when updating the flow.
---
Outside diff comments:
In `@Resgrid.Relay.Engine/Voice/RadioMode.cs`:
- Around line 29-56: Update RadioMode.RunAsync so MutableStatus.LiveKit is
always moved off Connecting and into a terminal state on every path. Right now
it only sets status.LiveKit to Connected after a successful JoinAsync, but the
seat-limit return, any join/connection failure, and the normal end of the method
can leave the last optimistic state visible. Use the existing status parameter
in RunAsync to set an appropriate disconnected/faulted state before each early
return and in failure/shutdown handling around provider.CanConnectAsync,
provider.GetChannelAsync, and manager.JoinAsync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34f72cf1-873f-4f63-9029-7f1f7834b20f
📒 Files selected for processing (20)
Resgrid.Audio.Core/ComService.csResgrid.Audio.Core/Radio/RadioBridge.csResgrid.Audio.Relay.Console/Program.csResgrid.Relay.Engine/RelayStatus.csResgrid.Relay.Engine/ServiceCollectionExtensions.csResgrid.Relay.Engine/Services/AudioImportService.csResgrid.Relay.Engine/Services/DispatchRelayService.csResgrid.Relay.Engine/Services/NotSupportedRelayService.csResgrid.Relay.Engine/Services/RadioRelayService.csResgrid.Relay.Engine/Services/RecordRelayService.csResgrid.Relay.Engine/Services/RelayServiceBase.csResgrid.Relay.Engine/Services/RelayServiceFactory.csResgrid.Relay.Engine/Services/SmtpRelayService.csResgrid.Relay.Engine/Smtp/SmtpTelemetry.csResgrid.Relay.Engine/Smtp/StatusReportingSmtpTelemetry.csResgrid.Relay.Engine/TuneSample.csResgrid.Relay.Engine/Voice/AudioImportMode.csResgrid.Relay.Engine/Voice/DispatchVoiceMode.csResgrid.Relay.Engine/Voice/RadioMode.csResgrid.Relay.Engine/Voice/RecordMode.cs
| SetReceiving(open); | ||
|
|
||
| if (!open || (_settings.AntiLoop && _transmitting)) | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only mark Receiving when RX audio is actually forwarded.
Line 139 uses raw squelch-open state, but Line 141 can still drop the frame because anti-loop is suppressing RX while TX is active. The new status surface then reports Receiving = true even though nothing is being published to the channel.
Suggested fix
bool open = IsSquelchOpen(frame);
- SetReceiving(open);
+ bool receiving = open && !(_settings.AntiLoop && _transmitting);
+ SetReceiving(receiving);
- if (!open || (_settings.AntiLoop && _transmitting))
+ if (!receiving)
return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SetReceiving(open); | |
| if (!open || (_settings.AntiLoop && _transmitting)) | |
| return; | |
| bool open = IsSquelchOpen(frame); | |
| bool receiving = open && !(_settings.AntiLoop && _transmitting); | |
| SetReceiving(receiving); | |
| if (!receiving) | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Audio.Core/Radio/RadioBridge.cs` around lines 139 - 142, Update
RadioBridge so Receiving is only set when RX audio is actually forwarded, not
just when squelch is open. In the receive path around SetReceiving/open and the
AntiLoop check, move or gate the Receiving state update so it happens only after
the early-return conditions pass, or clear it when _settings.AntiLoop suppresses
forwarding while _transmitting is true. Use the RadioBridge receive flow and
SetReceiving as the key symbols to adjust.
| await using (service) | ||
| { | ||
| await service.StartAsync(cancellationTokenSource.Token).ConfigureAwait(false); | ||
| } | ||
|
|
||
| return service.State == RelayServiceState.Faulted ? 1 : 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect relay service wrappers for non-zero RunAsync result handling.
# Expected: Each ExecuteAsync that awaits a mode RunAsync(...) stores/checks the int result
# and throws (or otherwise causes RelayServiceBase to transition to Faulted) when result != 0.
set -euo pipefail
fd -i '.*(RelayService|ImportService)\.cs$' . \
| xargs rg -n -C 6 'ExecuteAsync|RunAsync\(|returnCode|exitCode|!=\s*0|throw new'Repository: Resgrid/Relay
Length of output: 7645
🏁 Script executed:
find . -name "*.cs" -type f \( -name "AudioImportMode.cs" -o -name "SmtpRelayRunner.cs" -o -name "RecordMode.cs" -o -name "RadioMode.cs" -o -name "DispatchVoiceMode.cs" \) | xargs ast-grep outlineRepository: Resgrid/Relay
Length of output: 393
🏁 Script executed:
find . -name "*.cs" -type f | xargs rg -l "class.*RunAsync"Repository: Resgrid/Relay
Length of output: 155
🏁 Script executed:
rg -n "public static.*Task.*RunAsync" --type cs --files-with-matchesRepository: Resgrid/Relay
Length of output: 151
🏁 Script executed:
rg -l "class.*Mode" --type cs --max-count 10Repository: Resgrid/Relay
Length of output: 151
🏁 Script executed:
find . -name "*.cs" -type f -exec grep -l "RunAsync" {} \; | head -20Repository: Resgrid/Relay
Length of output: 699
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Voice/AudioImportMode.cs | head -60Repository: Resgrid/Relay
Length of output: 2812
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Voice/AudioImportMode.cs | tail -50Repository: Resgrid/Relay
Length of output: 2429
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Voice/RecordMode.csRepository: Resgrid/Relay
Length of output: 5671
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Voice/RadioMode.csRepository: Resgrid/Relay
Length of output: 12185
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Voice/DispatchVoiceMode.csRepository: Resgrid/Relay
Length of output: 5744
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Smtp/SmtpRelayRunner.csRepository: Resgrid/Relay
Length of output: 228
🏁 Script executed:
find . -name "SmtpRelayRunner.cs" -type fRepository: Resgrid/Relay
Length of output: 151
🏁 Script executed:
find . -name "*Smtp*.cs" -type fRepository: Resgrid/Relay
Length of output: 525
🏁 Script executed:
cat -n Resgrid.Relay.Engine/Services/SmtpRelayService.csRepository: Resgrid/Relay
Length of output: 2331
🏁 Script executed:
rg -l "class.*SmtpRelayRunner" --type csRepository: Resgrid/Relay
Length of output: 151
🏁 Script executed:
rg "SmtpRelayRunner" -t cs --type-limit 1 --files-with-matchesRepository: Resgrid/Relay
Length of output: 593
Return values from mode runners are ignored, masking failures
The relay service wrappers (AudioImportService, RecordRelayService, RadioRelayService, DispatchRelayService) await their respective mode RunAsync methods without checking the returned int.
While AudioImportMode, RecordMode, RadioMode, and DispatchVoiceMode all return Task<int> (returning non-zero codes for specific failures like seat limits or missing configuration), the services discard this value. Since RelayServiceBase only marks the state as Faulted if an exception is thrown, a non-zero return code is currently treated as a successful shutdown (exit code 0).
Ensure wrappers explicitly check the returned status and throw if it is non-zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Audio.Relay.Console/Program.cs` around lines 102 - 107, The relay
service wrappers are ignoring the non-zero status returned by the mode runners,
so failures from methods like RunAsync on AudioImportService,
RecordRelayService, RadioRelayService, and DispatchRelayService are being
treated as success. Update the service wrapper flow around
RelayServiceBase.StartAsync / the mode RunAsync calls to capture the returned
int, and if it is non-zero, throw an exception so the service is marked faulted
and the process exits with a failure code instead of returning 0.
| protected override async Task ExecuteAsync(CancellationToken token) | ||
| { | ||
| MutableStatus.LiveKit = ConnectionState.Connecting; | ||
| MutableStatus.Tts = ConnectionState.Connecting; | ||
| await DispatchVoiceMode.RunAsync(Options, Logger, token, MutableStatus).ConfigureAwait(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Surface non-zero dispatch exits as faults.
Line 24 drops DispatchVoiceMode.RunAsync's return code. That mode already returns 1 for validation/configuration failures in Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs:22-36, while Resgrid.Audio.Relay.Console/Program.cs:88-108 only returns a non-zero process exit when service.State == RelayServiceState.Faulted. Right now those failures look like a clean stop.
Suggested fix
protected override async Task ExecuteAsync(CancellationToken token)
{
MutableStatus.LiveKit = ConnectionState.Connecting;
MutableStatus.Tts = ConnectionState.Connecting;
- await DispatchVoiceMode.RunAsync(Options, Logger, token, MutableStatus).ConfigureAwait(false);
+ var exitCode = await DispatchVoiceMode.RunAsync(Options, Logger, token, MutableStatus).ConfigureAwait(false);
+ if (exitCode != 0)
+ throw new System.InvalidOperationException($"Dispatch relay exited with code {exitCode}.");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected override async Task ExecuteAsync(CancellationToken token) | |
| { | |
| MutableStatus.LiveKit = ConnectionState.Connecting; | |
| MutableStatus.Tts = ConnectionState.Connecting; | |
| await DispatchVoiceMode.RunAsync(Options, Logger, token, MutableStatus).ConfigureAwait(false); | |
| protected override async Task ExecuteAsync(CancellationToken token) | |
| { | |
| MutableStatus.LiveKit = ConnectionState.Connecting; | |
| MutableStatus.Tts = ConnectionState.Connecting; | |
| var exitCode = await DispatchVoiceMode.RunAsync(Options, Logger, token, MutableStatus).ConfigureAwait(false); | |
| if (exitCode != 0) | |
| throw new System.InvalidOperationException($"Dispatch relay exited with code {exitCode}."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Services/DispatchRelayService.cs` around lines 20 - 24,
`DispatchRelayService.ExecuteAsync` is ignoring the return value from
`DispatchVoiceMode.RunAsync`, so validation/configuration failures are not
marked as faults. Capture the return code from `RunAsync` and, when it is
non-zero, transition the relay/service state to faulted (or otherwise signal
failure through the existing state mechanism) before exiting `ExecuteAsync`. Use
the existing `MutableStatus`/`ConnectionState` flow in `DispatchRelayService`
and the `DispatchVoiceMode.RunAsync` return contract to keep behavior aligned
with `Program`’s `RelayServiceState.Faulted` handling.
| public async Task StartAsync(CancellationToken token) | ||
| { | ||
| _cts = CancellationTokenSource.CreateLinkedTokenSource(token); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject re-entrant starts on the same service instance.
Line 48 replaces _cts on every StartAsync call, but there is no state check preventing a second start while the first ExecuteAsync is still running. That leaves the original run uncancellable via StopAsync and can drive two mode loops through one service instance.
Suggested fix
public async Task StartAsync(CancellationToken token)
{
- _cts = CancellationTokenSource.CreateLinkedTokenSource(token);
+ lock (_sync)
+ {
+ if (_state != RelayServiceState.Stopped)
+ throw new InvalidOperationException($"Cannot start relay service from state {_state}.");
+
+ _cts?.Dispose();
+ _cts = CancellationTokenSource.CreateLinkedTokenSource(token);
+ }
TransitionTo(RelayServiceState.Starting);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task StartAsync(CancellationToken token) | |
| { | |
| _cts = CancellationTokenSource.CreateLinkedTokenSource(token); | |
| public async Task StartAsync(CancellationToken token) | |
| { | |
| lock (_sync) | |
| { | |
| if (_state != RelayServiceState.Stopped) | |
| throw new InvalidOperationException($"Cannot start relay service from state {_state}."); | |
| _cts?.Dispose(); | |
| _cts = CancellationTokenSource.CreateLinkedTokenSource(token); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Services/RelayServiceBase.cs` around lines 46 - 48,
Prevent re-entrant StartAsync calls on the same RelayServiceBase instance by
adding an explicit running/state check before replacing _cts. Update StartAsync
and the service lifecycle in RelayServiceBase so a second start either returns
early or throws a clear error when ExecuteAsync is already active, and only
create a new linked CancellationTokenSource after confirming the service is not
already running.
| catch (OperationCanceledException) | ||
| { | ||
| TransitionTo(RelayServiceState.Stopped); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only treat cancellation as graceful when this service was actually canceled.
Line 56 currently maps any OperationCanceledException to Stopped. If ExecuteAsync or one of its dependencies throws that exception for its own timeout/path, State never becomes Faulted, and the console path shown in Resgrid.Audio.Relay.Console/Program.cs:88-107 will still return success.
Suggested fix
- catch (OperationCanceledException)
+ catch (OperationCanceledException) when (_cts?.IsCancellationRequested == true || token.IsCancellationRequested)
{
TransitionTo(RelayServiceState.Stopped);
}
+ catch (OperationCanceledException ex)
+ {
+ Logger?.Error(ex, "Relay mode '{Mode}' canceled unexpectedly", Mode);
+ TransitionTo(RelayServiceState.Faulted, ex.Message);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (OperationCanceledException) | |
| { | |
| TransitionTo(RelayServiceState.Stopped); | |
| } | |
| catch (OperationCanceledException) when (_cts?.IsCancellationRequested == true || token.IsCancellationRequested) | |
| { | |
| TransitionTo(RelayServiceState.Stopped); | |
| } | |
| catch (OperationCanceledException ex) | |
| { | |
| Logger?.Error(ex, "Relay mode '{Mode}' canceled unexpectedly", Mode); | |
| TransitionTo(RelayServiceState.Faulted, ex.Message); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Services/RelayServiceBase.cs` around lines 56 - 59, The
catch in RelayServiceBase.ExecuteAsync is treating every
OperationCanceledException as a graceful stop, which can hide real faults.
Update the exception handling in RelayServiceBase so it only transitions to
RelayServiceState.Stopped when the service’s own cancellation token was actually
requested; otherwise let the failure flow to Faulted. Use the existing
ExecuteAsync flow and TransitionTo method to distinguish genuine shutdown
cancellation from dependency timeouts or other canceled operations, so the
console path in Program can surface failures correctly.
| if (options == null) | ||
| throw new ArgumentNullException(nameof(options)); | ||
|
|
||
| var mode = (options.Mode ?? "smtp").Trim().ToLowerInvariant(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Default blank modes to smtp.
Line 20 only falls back when options.Mode is null. An empty or whitespace config value becomes "" after Trim() and hits the unsupported-mode path instead of the intended default.
Suggested fix
- var mode = (options.Mode ?? "smtp").Trim().ToLowerInvariant();
+ var mode = string.IsNullOrWhiteSpace(options.Mode)
+ ? "smtp"
+ : options.Mode.Trim().ToLowerInvariant();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var mode = (options.Mode ?? "smtp").Trim().ToLowerInvariant(); | |
| var mode = string.IsNullOrWhiteSpace(options.Mode) | |
| ? "smtp" | |
| : options.Mode.Trim().ToLowerInvariant(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Services/RelayServiceFactory.cs` at line 20, The mode
normalization in RelayServiceFactory should default blank or whitespace values
to smtp, not just null. Update the logic around the mode assignment so that
options.Mode is checked for null, empty, or whitespace before trimming/lowering,
and fall back to smtp in those cases; keep the existing unsupported-mode
handling for real values, using the mode variable in RelayServiceFactory as the
central lookup point.
| MutableStatus.Redis = Options.Smtp.RedisCache?.Enabled == true | ||
| ? ConnectionState.Connecting | ||
| : ConnectionState.NotApplicable; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't leave Redis status stuck in Connecting.
Line 45 sets MutableStatus.Redis to Connecting when Redis is enabled, but nothing in this service or StatusReportingSmtpTelemetry ever updates it afterward. That means the Windows status surface will show a perpetual in-between state instead of a real Redis status. Either perform a real probe here and set Connected/Disconnected, or leave it non-transitional until you have an actual Redis signal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Services/SmtpRelayService.cs` around lines 45 - 47, The
Redis status in SmtpRelayService is being set to the transitional Connecting
state without any follow-up update, so the status surface can stay stuck. Update
the MutableStatus.Redis assignment in SmtpRelayService to either perform an
actual Redis probe and set a final state such as Connected or Disconnected, or
keep it non-transitional (for example NotApplicable) until
StatusReportingSmtpTelemetry or another Redis signal can provide a real result.
| // The Resgrid API client is built and ready to be used. | ||
| if (status != null) | ||
| status.ResgridApi = ConnectionState.Connected; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only mark Resgrid API connected after the health check succeeds.
Line 41 sets status.ResgridApi to Connected before IsConnectionValid() runs, so a failed API health check returns 1 while the UI/status model still says the API is connected.
Proposed fix
- // The Resgrid API client is built and ready to be used.
- if (status != null)
- status.ResgridApi = ConnectionState.Connected;
-
var audioStorage = new WatcherAudioStorage(logger);
var evaluator = new AudioEvaluator(logger);
var recorder = new AudioRecorder(evaluator, audioStorage);
@@
if (!comService.IsConnectionValid())
{
logger.Error("Unable to reach the Resgrid v4 API with the configured OpenID Connect settings.");
return 1;
}
+
+ // The Resgrid API health check passed.
+ if (status != null)
+ status.ResgridApi = ConnectionState.Connected;Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Voice/AudioImportMode.cs` around lines 39 - 41, The
Resgrid API connection state is being marked as Connected too early in
AudioImportMode, before the health check completes. Update the flow in the code
around IsConnectionValid() so status.ResgridApi is only set to
ConnectionState.Connected after the health check succeeds, and keep it unset or
non-connected when validation fails; apply the same correction wherever this
connection-status assignment appears in the import path.
| using var tts = new ResgridTtsClient(options.Tts, logger); | ||
|
|
||
| // TTS client created — the Resgrid TTS service is reachable. | ||
| if (status != null) | ||
| status.Tts = ConnectionState.Connected; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t mark TTS connected just from constructing the client.
ResgridTtsClient does not contact the TTS service in its constructor; the first network call happens during synthesis. This can show TTS as connected even when the URL is unreachable.
Minimal fix option
using var tts = new ResgridTtsClient(options.Tts, logger);
-
- // TTS client created — the Resgrid TTS service is reachable.
- if (status != null)
- status.Tts = ConnectionState.Connected;
var service = new DispatchToneOutService(tts, new ToneGenerator(), options.DispatchVoice.Tone, logger);
@@
await service.AnnounceAsync(publisher, text, cancellationToken).ConfigureAwait(false);
+ if (status != null)
+ status.Tts = ConnectionState.Connected;
// Only mark the call handled once the announcement actually succeeds,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs` around lines 55 - 59, Don’t
set TTS to Connected immediately after constructing ResgridTtsClient in
DispatchVoiceMode because the constructor does not verify the service is
reachable. Move the connection-state update to the first successful
synthesis/health-check path in DispatchVoiceMode, using the ResgridTtsClient
instance only after a real network call succeeds, and keep status.Tts unchanged
until that check passes.
| await using var bridge = new RadioBridge(device, ptt, carrier, radioSettings, logger, mdc, emergency, alertSink); | ||
| await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Mirror the bridge's live TX/RX state onto the status surface. | ||
| // note: squelch open-state and input dBFS live inside RadioBridge in radio mode | ||
| // and are not exposed, so InputDbfs/SquelchOpen are intentionally left unwired here. | ||
| if (status != null) | ||
| bridge.TxRxChanged += (_, __) => | ||
| { | ||
| status.Transmitting = bridge.Transmitting; | ||
| status.Receiving = bridge.Receiving; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Subscribe before startup or seed the initial TX/RX snapshot.
The handler is attached after bridge.StartAsync(...) returns. If the bridge is already keyed or receiving during startup, no edge is raised and status.Transmitting / status.Receiving stay false until the next transition.
Suggested fix
await using var bridge = new RadioBridge(device, ptt, carrier, radioSettings, logger, mdc, emergency, alertSink);
- await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false);
-
- // Mirror the bridge's live TX/RX state onto the status surface.
- // note: squelch open-state and input dBFS live inside RadioBridge in radio mode
- // and are not exposed, so InputDbfs/SquelchOpen are intentionally left unwired here.
if (status != null)
bridge.TxRxChanged += (_, __) =>
{
status.Transmitting = bridge.Transmitting;
status.Receiving = bridge.Receiving;
};
+
+ await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false);
+
+ if (status != null)
+ {
+ status.Transmitting = bridge.Transmitting;
+ status.Receiving = bridge.Receiving;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await using var bridge = new RadioBridge(device, ptt, carrier, radioSettings, logger, mdc, emergency, alertSink); | |
| await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false); | |
| // Mirror the bridge's live TX/RX state onto the status surface. | |
| // note: squelch open-state and input dBFS live inside RadioBridge in radio mode | |
| // and are not exposed, so InputDbfs/SquelchOpen are intentionally left unwired here. | |
| if (status != null) | |
| bridge.TxRxChanged += (_, __) => | |
| { | |
| status.Transmitting = bridge.Transmitting; | |
| status.Receiving = bridge.Receiving; | |
| }; | |
| await using var bridge = new RadioBridge(device, ptt, carrier, radioSettings, logger, mdc, emergency, alertSink); | |
| if (status != null) | |
| bridge.TxRxChanged += (_, __) => | |
| { | |
| status.Transmitting = bridge.Transmitting; | |
| status.Receiving = bridge.Receiving; | |
| }; | |
| await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false); | |
| if (status != null) | |
| { | |
| status.Transmitting = bridge.Transmitting; | |
| status.Receiving = bridge.Receiving; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Resgrid.Relay.Engine/Voice/RadioMode.cs` around lines 68 - 80, The TX/RX
status wiring in RadioMode is happening too late because the TxRxChanged
subscription is added after RadioBridge.StartAsync, so the initial
transmitting/receiving state can be missed. Move the status handler setup before
calling bridge.StartAsync, or explicitly seed status.Transmitting and
status.Receiving from the bridge’s current state right after startup. Use the
RadioMode method and RadioBridge.TxRxChanged/StartAsync as the key symbols when
updating the flow.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes